-
Notifications
You must be signed in to change notification settings - Fork 67
Make JNI handling of Int/UInt match FFM mode. #460
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Thanks for this! Definitely an important step that we handle I do however, think we need to give this some more thought. Especially since the JNI mode will probably be used a lot by Android folks, which is commonly 32-bit. If you have the following Swift function: func f(i: Int)with this PR it gets translated to void f(long i)however, the downcall to Swift would be Also we should add some codegen tests and runtime tests while we are it, as well as add the |
|
I am aware of Int64 -> Int32 conversion problem but I didn't think about it as problem needed to be addressed in the same PR. To be honest I thought 32-bit is rare for Android, so I have no problem to work on solution for it in this PR. |
|
Thanks for the PR, this would be good to address. Overall the two "modes" should behave the same wherever possible and it is a problem that they're a bit independent currently so this would be good to align the two. 32bit is very rare and AFAICS there is a number of prominent apps that choose to not support 32bit Android devices. The Java annotation mentioned is the https://github.com/swiftlang/swift-java/blob/main/SwiftKitCore/src/main/java/org/swift/swiftkit/core/annotations/Unsigned.java so please have a look how we're using it in the ffm mode to mark something was an "U"Int. The imported parameters or return types basically get an |
|
I know 32-bit is rare these days. But since it's still a supported platform, for both Swift and the Swift Android SDK, I think we should still handle it. For example for us, most of our devices are 32-bit, and that is in the millions:) OK by me to merge this and we can tackle it in another PR, just wanted to flag this, as we need to handle it somehow. @_cdecl()
func $f(i: jlong) {
let swiftInt = Int64(fromJNI: i, ...)
#if _pointerBitWidth(_32)
guard swiftInt <= Int32.max, swiftInt >= Int32.min else {
// throw overflow exception
}
#endif
}maybe this could even just be in the |
|
Well we can't merge anything this week, so yes, let's please polish this up before branches get unlocked next week. |
…, implemented besic check for overflow of Int/UInt.
|
I committed small change with a proposed solution for check and basic tests. Let me know what you think, and if the response is positive, I'll cover rest of JNI cases(enum cases etc) and apply it to FFM. |
ktoso
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks quite neat, thank you.
Can you add some runtime tests please?
| } | ||
| } | ||
|
|
||
| static func indirectConversionSetepSwiftType(for knownKind: SwiftKnownTypeDeclKind, from knownTypes: SwiftKnownTypes) -> SwiftType? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
likely indirectConversionStepSwiftType?
| //===----------------------------------------------------------------------===// | ||
|
|
||
| /// Describes a Java exception class (e.g. `SwiftIntegerOverflowException`) | ||
| public struct JavaException: Equatable, Hashable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably need to call this JavaExceptionType so we don't confuse ourselfes with a JavaExpcetion that would be a @javaclass
|
|
||
| package org.swift.swiftkit.core; | ||
|
|
||
| public class SwiftIntegerOverflowException extends RuntimeException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add some docs on it, the message is pretty good but we should have similar explanation in docs
| #if _pointerBitWidth(_32) | ||
| guard indirect_newValue >= UInt32.min && indirect_newValue <= UInt32.max else { | ||
| environment.throwJavaException(javaException: .integerOverflow) | ||
| return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include the #endif in this chunk please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added endif in next chunk. I wanted to remove closing breace from this chunk as it contains comment generated by printer about file and line of invokation which isn't nice for those assertions.
|
|
||
| // Make indirect variables | ||
| for (name, lowered) in indirectVariables { | ||
| printer.print("let indirect_\(name) = \(lowered)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have a naming convention for variables to suffix with $ so we don't clash with user defined variables; It's very unlikely anyone would do some "indirect_bla" but please let's be consistent.
This could be name$indirect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I like name$indirect
| let conversion: NativeSwiftConversionStep | ||
|
|
||
| /// Represents swift type for indirect variable used in required checks, e.g Int64 for Int overflow check on 32-bit platforms | ||
| let indirectConversion: NativeSwiftConversionStep? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe let's add that "This will introduce a new name$indirect variabler" so it is easier to mentally connect the two? The "indirect" name sounds good!
| let indirectConversion: NativeSwiftConversionStep? | ||
|
|
||
| /// Represents check operations executed in if/guard conditional block for check during conversion | ||
| let conversionCheck: NativeSwiftConversionCheck? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nicely integrated here 👍
| printer.printBraceBlock("guard \(check) else") { printer in | ||
| let dummyReturn = dummyReturn(for: nativeSignature) | ||
| printer.print("environment.throwJavaException(javaException: .integerOverflow)") | ||
| printer.print(dummyReturn) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd skip the dummyreturn variable:
| printer.print(dummyReturn) | |
| printer.print(dummyReturn(for: nativeSignature)) |
|
This looks great! I only have one comment regarding the implementation for maintainability. Up until now all conversions have just been done in the Was there a specific reason for introducing the "indirect" and "check" types? And not just doing it inside a |
|
Firstly, thank you @madsodgaard for sharing your concern :)
Of course, but I realise that for others this may not be a sufficient reason. As someone who just started to contribute I felt |
I believe that both modes should behave similarly when handling Int/UInt types. This PR makes JNI handle these types in the same way as FFM.